Skip to content

Ruleset: (CLI) <arg> settings in higher level ruleset(s) overrule args set in included ruleset(s) #1005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Apr 15, 2025

Description

Again, this is a completely different solution to issues squizlabs/PHP_CodeSniffer#2395, squizlabs/PHP_CodeSniffer#2597, squizlabs/PHP_CodeSniffer#2602, compared to the original solution which was previously committed to the old 4.0 branch in response to issue squizlabs/PHP_CodeSniffer#2197.

The "old' solution was to process rulesets "top to bottom", i.e. every directive is processed when "read" while recursing through the rulesets. I've evaluated that solution carefully and found it counter-intuitive, which means that I expect that it would raise lots of support questions. I also expect that solution to break way more rulesets than is justified to solve these issues.

The original solution would require ruleset maintainers to have a high awareness of what CLI args are being set in included rulesets. Included rulesets are often PHPCS native standards or external standards, which are both subject to change in every new release. With ruleset processing being top-to-bottom, any "must have" <arg> settings would have to be set before any include which could possibly also have that directive set. This also means that an external standard adding a CLI arg (like tab-width) to one of their rulesets, would become a potentially breaking change, as a "consumer" ruleset may not realize they were relying on a default value for a "must have" CLI flag and that value being changed in an included ruleset could now break their CS scans.

So... having said that, this commit contains an alternative, far more specific, and far less invasive, solution for the originally posed problem.


This commit changes the processing of arg directives to be based on the inclusion depth, with the highest level ruleset setting a CLI arg value overruling the value for that arg as set in lower level (included) rulesets.

When two rulesets at the same "level" both set the same arg directive, the first one included "wins".

👉 Note: this is different from <config> directives where the last included ruleset wins. However, it means that for two rulesets at the same "level", the behaviour is unchanged, which makes the BC-break smaller.

To illustrate:

File: phpcs.xml.dist
Sets arg `tab-width` to `4`
Includes CompanyRulesetA

File CompanyRulesetA/ruleset.xml
Sets arg `tab-width` to `2`

In 3.x: Config::$tabWidth would be set to 2
In 4.x: Config::$tabWidth will be set to 4 (higher level ruleset wins)

File: phpcs.xml.dist
Includes CompanyRulesetA and AnotherRuleset

File CompanyRulesetA/ruleset.xml
Sets arg `extensions` to `php,module,phpt`.

File AnotherRuleset/ruleset.xml
Sets arg `extensions` to `php,module,profile,test`

In 3.x: Config::$extensions would be set to php,module,phpt
In 4.x: Config::$extensions will be set to php,module,phpt (CompanyRulesetA and AnotherRuleset are at the same inclusion depth, first one wins)

The solution as implemented takes CLI flags which can be set using multiple different CLI arguments into account. What this means is as follows:

File: phpcs.xml.dist
Sets arg `no-colors`
Includes CompanyRulesetA

File CompanyRulesetA/ruleset.xml
Sets arg `colors`

In 3.x: Config::$colors would be set to true
In 4.x: Config::$colors will be set to false (higher level ruleset wins)

Includes ample tests.

Note: there are no tests covering the cache setting, while by rights, there should be, but this is (yet again) something where the global PHP_CODESNIFFER_IN_TESTS is checked from within the runtime code, which means that the cache directive cannot be set/changed from within the test suite. Yes, this should be changed, but that is outside of the scope of this PR. For now, this has been manually tested and found working. See #966.

Includes minor tweak to improve the debug information and only display that something was set when it actually has been set.

Suggested changelog entry

  • When processing rulesets, <arg> directives will be applied based on the nesting level of the ruleset.
    • Previously, it was not possible to overrule a <arg> directive set in an included ruleset from the "root" ruleset.
    • Now, <arg> directives set in the "root" ruleset will always "win" over directives in included rulesets.
    • When two included rulesets at the same nesting level both set the same directive, the value from the first included ruleset "wins" (= same as before).

Related issues/external references

Fixes squizlabs/PHP_CodeSniffer#2395
Fixes squizlabs/PHP_CodeSniffer#2597
Fixes squizlabs/PHP_CodeSniffer#2602

…rgs set in included ruleset(s)

Again, this is a completely different solution to issues squizlabs/PHP_CodeSniffer 2395, 2597, 2602, compared to the original solution which was previously committed to the old 4.0 branch in response to issue 2197.

The "old' solution was to process rulesets "top to bottom", i.e. every directive is processed when "read" while recursing through the rulesets.
I've evaluated that solution carefully and found it counter-intuitive, which means that I expect that it would raise lots of support questions. I also expect that solution to break way more rulesets than is justified to solve these issues.

The original solution would require ruleset maintainers to have a high awareness of what CLI args are being set in included rulesets. Included rulesets are often PHPCS native standards or external standards, which are both subject to change in every new release.
With ruleset processing being top-to-bottom, any "must have" `<arg>` settings would have to be set _before_ any include which could possibly also have that directive set. This also means that an external standard _adding_ a CLI arg (like `tab-width`) to one of their rulesets, would become a potentially breaking change, as a "consumer" ruleset may not realize they were relying on a default value for a "must have" CLI flag and that value being changed in an included ruleset could now break their CS scans.

So... having said that, this commit contains an alternative, far more specific, and far less invasive, solution for the originally posed problem.

---

This commit changes the processing of `arg` directives to be based on the inclusion depth, with the highest level ruleset setting a CLI arg value overruling the value for that arg as set in lower level (included) rulesets.

When two rulesets at the same "level" both set the same `arg` directive, the first one included "wins".

:point_right: Note: this is different from `<config>` directives where the **_last_** included ruleset wins. However, it means that for two rulesets at the same "level", the behaviour is unchanged, which makes the BC-break smaller.

To illustrate:
```
File: phpcs.xml.dist
Sets arg `tab-width` to `4`
Includes CompanyRulesetA

File CompanyRulesetA/ruleset.xml
Sets arg `tab-width` to `2`
```

In 3.x: `Config::$tabWidth` would be set to `2`
In 4.x: `Config::$tabWidth` will be set to `4` (higher level ruleset wins)

```
File: phpcs.xml.dist
Includes CompanyRulesetA and AnotherRuleset

File CompanyRulesetA/ruleset.xml
Sets arg `extensions` to `php,module,phpt`.

File AnotherRuleset/ruleset.xml
Sets arg `extensions` to `php,module,profile,test`
```

In 3.x: `Config::$extensions` would be set to `php,module,phpt`
In 4.x: `Config::$extensions` will be set to `php,module,phpt` (CompanyRulesetA and AnotherRuleset are at the same inclusion depth, first one wins)

The solution as implemented takes CLI flags which can be set using multiple different CLI arguments into account.
What this means is as follows:
```
File: phpcs.xml.dist
Sets arg `no-colors`
Includes CompanyRulesetA

File CompanyRulesetA/ruleset.xml
Sets arg `colors`
```
In 3.x: `Config::$colors` would be set to `true`
In 4.x: `Config::$colors` will be set to `false` (higher level ruleset wins)

Includes ample tests.

Note: there are no tests covering the `cache` setting, while by rights, there should be, but this is (yet again) something where the global `PHP_CODESNIFFER_IN_TESTS` is checked from within the runtime code, which means that the `cache` directive cannot be set/changed from within the test suite.
Yes, this should be changed, but that is outside of the scope of this PR. For now, this has been manually tested and found working.

Includes minor tweak to improve the debug information and only display that something was set when it actually has been set.

Fixes squizlabs/PHP_CodeSniffer 2395
Fixes squizlabs/PHP_CodeSniffer 2597
Fixes squizlabs/PHP_CodeSniffer 2602
@jrfnl jrfnl merged commit b63cedf into 4.x Apr 15, 2025
62 checks passed
@jrfnl jrfnl deleted the phpcs-4.0/feature/sq-2395-2597-2602-ruleset-config-arg-processing branch April 15, 2025 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants